feat(vortex-datafusion): struct scalar conversion + extension-over-struct scan - #8453
Conversation
Signed-off-by: Nemo Yu <zyu379@wisc.edu>
c5f96ff to
ffbbfdd
Compare
|
|
||
| /// The struct fields of `dtype` if it is a struct, or of an extension type whose storage is | ||
| /// (eventually) a struct -- e.g. the native geo Point over `Struct<x, y>`. | ||
| fn struct_fields(dtype: &DType) -> Option<&StructFields> { |
There was a problem hiding this comment.
nit - this is only used once, can we just inline it?
Merging this PR will not alter performance
|
| Mode | Benchmark | BASE |
HEAD |
Efficiency | |
|---|---|---|---|---|---|
| ❌ | Simulation | chunked_bool_canonical_into[(1000, 10)] |
20.6 µs | 35.7 µs | -42.29% |
| ❌ | Simulation | chunked_dict_primitive_into_canonical[u32, (1000, 10, 10)] |
120.7 µs | 183 µs | -34.02% |
| ❌ | Simulation | encode_varbin[(1000, 2)] |
176.1 µs | 237.1 µs | -25.74% |
| ❌ | Simulation | chunked_varbinview_canonical_into[(1000, 10)] |
161.8 µs | 198.1 µs | -18.29% |
| ❌ | Simulation | chunked_varbinview_into_canonical[(1000, 10)] |
177.1 µs | 214 µs | -17.25% |
| ❌ | Simulation | bench_many_codes_few_values[1024] |
393.2 µs | 465.6 µs | -15.54% |
| ❌ | Simulation | decompress_rd[f64, (100000, 0.0)] |
845.5 µs | 982.8 µs | -13.97% |
| ❌ | Simulation | varbinview_large |
112.2 µs | 130.4 µs | -13.97% |
| ❌ | Simulation | chunked_varbinview_canonical_into[(100, 100)] |
273.8 µs | 308.8 µs | -11.33% |
| ❌ | Simulation | chunked_varbinview_into_canonical[(100, 100)] |
326.4 µs | 364.9 µs | -10.55% |
| ⚡ | Simulation | sum_i32_nullable_all_valid |
69.2 µs | 35.4 µs | +95.64% |
| ⚡ | Simulation | null_count_run_end[(10000, 4, 0.01)] |
125.4 µs | 91.6 µs | +36.98% |
| ⚡ | Simulation | encode_varbinview[(1000, 2)] |
189 µs | 157.1 µs | +20.26% |
| ⚡ | Simulation | take_10k_contiguous |
252.8 µs | 218.1 µs | +15.89% |
| ⚡ | Simulation | and_bool_nullable |
93.7 µs | 82.7 µs | +13.29% |
| ⚡ | Simulation | baseline_lt[4, 1024] |
78.5 µs | 69.6 µs | +12.76% |
| ⚡ | Simulation | decompress_rd[f64, (100000, 0.01)] |
981.2 µs | 890.4 µs | +10.2% |
| ⚡ | Simulation | decompress_rd[f64, (100000, 0.1)] |
981.2 µs | 890.4 µs | +10.19% |
| 🆕 | Simulation | cast_i32_to_u32[65536] |
N/A | 840.3 µs | N/A |
| 🆕 | Simulation | cast_u32_to_u8[65536] |
N/A | 251.5 µs | N/A |
| ... | ... | ... | ... | ... | ... |
ℹ️ Only the first 20 benchmarks are displayed. Go to the app to view all benchmarks.
Tip
Investigate this regression by commenting @codspeedbot fix this regression on this PR, or directly use the CodSpeed MCP with your agent.
Comparing nemo/geo-q1 (4a4a4f4) with develop (679e2c5)2
Footnotes
-
38 benchmarks were skipped, so the baseline results were used instead. If they were deleted from the codebase, click here and archive them to remove them from the performance reports. ↩
-
No successful run was found on
develop(f0b8fb9) during the generation of this report, so 679e2c5 was used instead as the comparison base. There might be some changes unrelated to this pull request in this report. ↩
| DType::Struct(struct_fields, _) => { | ||
| let scalar = self.as_struct(); | ||
| let (fields, arrays): (Vec<Field>, Vec<_>) = struct_fields |
There was a problem hiding this comment.
nit: might as well split this whole branch out into a separate function?
| } | ||
| ScalarValue::Dictionary(_, v) => Scalar::from_df(v.as_ref()), | ||
| ScalarValue::Struct(array) => { | ||
| let nullable = array.is_null(0); |
There was a problem hiding this comment.
the struct array here has a DataType with the nullability info, I think using it is much clearer.
There was a problem hiding this comment.
ok reading down this is not nullable, this is an actual check if its null.
There was a problem hiding this comment.
I think you're using it for both, it should be two different things, one derived from the DataType and one from the data itself.
| .map(|column| { | ||
| Scalar::from_df( | ||
| &ScalarValue::try_from_array(&**column, 0).unwrap_or_else(|e| { | ||
| unimplemented!( |
There was a problem hiding this comment.
Should return an error, unimplemented is semantically code that hasn't been implemented yet (and it panics).
There was a problem hiding this comment.
from_df is infallible by design — fn from_df(value: &ScalarValue) -> Scalar returns a
Scalar, not a Result — so we can't return an Err here. For the (effectively unreachable)
"struct child can't convert" case, a panic is the only option.
| .iter() | ||
| .map(|column| { | ||
| Scalar::from_df( | ||
| &ScalarValue::try_from_array(&**column, 0).unwrap_or_else(|e| { |
There was a problem hiding this comment.
&**column - might be correct, but there's almost certainly a more idiomatic way to express that.
Signed-off-by: Nemo Yu <zyu379@wisc.edu>
Signed-off-by: Nemo Yu <zyu379@wisc.edu>
eaa9ca7 to
f52a2c4
Compare
4 participants
Summary
Testing
add null/non-null struct scalar round-trips.